JIT: cleanup redundant exact type tests by AndyAyersMS · Pull Request #27397 · dotnet/coreclr

AndyAyersMS · 2019-10-23T19:00:16Z

Generate exact type assertions from exact type tests in the IR. Look for these
assertions and set the value number for RELOPs with known outcomes. Process
JTRUE nodes in the main assertion prop optimization loop.

This combination of changes removes residual exact type tests from cast
expansions when they are anticipated by user inserted exact type tests, as in:

    if (b.GetType() == typeof(D))
    {
        return (D)b;
    }

Closes #14471.

Generate exact type assertions from exact type tests in the IR. Look for these assertions and set the value number for RELOPs with known outcomes. Process JTRUE nodes in the main assertion prop optimization loop. This combination of changes removes residual exact type tests from cast expansions when they are anticipated by user inserted exact type tests, as in: ```C# if (b.GetType() == typeof(D)) { return (D)b; } ``` Closes #14471.

AndyAyersMS · 2019-10-23T19:02:38Z

@dotnet/jit-contrib PTAL

diffs:

Summary:
(Lower is better)

Total bytes of diff: -2080 (-0.01% of base)
    diff is an improvement.

Top file improvements by size (bytes):
        -432 : xunit.assert.dasm (-0.35% of base)
        -432 : xunit.core.dasm (-0.66% of base)
        -432 : xunit.execution.dotnet.dasm (-0.19% of base)
        -318 : System.Data.Common.dasm (-0.02% of base)
        -221 : System.Security.Cryptography.X509Certificates.dasm (-0.13% of base)

8 total files with size differences (8 improved, 0 regressed), 121 unchanged.

Top method improvements by size (bytes):
        -318 (-28.22% of base) : System.Data.Common.dasm - BigIntegerStorage:ConvertToBigInteger(ref,ref):struct
        -130 (-2.22% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(ref,ref):this (14 methods)
         -97 (-2.11% of base) : System.Security.AccessControl.dasm - CommonAcl:RemoveQualifiedAces(ref,int,int,ubyte,bool,int,struct,struct):bool:this
         -72 (-4.89% of base) : xunit.assert.dasm - AssertEqualityComparer`1:Equals(ubyte,ubyte):bool:this
         -72 (-4.82% of base) : xunit.assert.dasm - AssertEqualityComparer`1:Equals(short,short):bool:this

Top method improvements by size (percentage):
        -318 (-28.22% of base) : System.Data.Common.dasm - BigIntegerStorage:ConvertToBigInteger(ref,ref):struct
         -36 (-27.07% of base) : System.Security.Cryptography.X509Certificates.dasm - FindPal:ConfirmedCast(ref):int
         -36 (-26.87% of base) : System.Security.Cryptography.X509Certificates.dasm - FindPal:ConfirmedCast(ref):ubyte
         -36 (-26.87% of base) : System.Security.Cryptography.X509Certificates.dasm - FindPal:ConfirmedCast(ref):long
         -41 (-26.80% of base) : System.Security.Cryptography.X509Certificates.dasm - FindPal:ConfirmedCast(ref):struct

28 total methods with size differences (28 improved, 0 regressed), 203097 unchanged.

on an example like the one in #14471, we now generate:

G_M62121_IG01:

G_M62121_IG02:
       mov      rax, 0xD1FFAB1E
       cmp      qword ptr [rcx], rax
       jne      SHORT G_M62121_IG05

G_M62121_IG03:		;; bbWeight=0.50
       mov      rax, rcx

G_M62121_IG04:		;; bbWeight=0.50
       ret      

G_M62121_IG05:		;; bbWeight=0.50
       xor      rax, rax

G_M62121_IG06:		;; bbWeight=0.50
       ret

mikedn · 2019-10-23T19:07:10Z

src/jit/assertionprop.cpp

+        {
+            GenTreeIndir* indir = op1->AsIndir();
+
+            if (!indir->HasIndex() && (indir->Offset() == 0))


I don't think you should use GenTreeIndir's HasIndex, Base and Offset. These are practically meaningless before lowering because they depend on containment and address modes. Before lowering:

HasIndex() is always false

Base() is just Addr()

Offset() is always 0
Basically these shouldn't exist on GenTreeIndir, they're misleading.

Good point, I can remove these.

mikedn · 2019-10-23T19:11:32Z

src/jit/assertionprop.cpp

            op1->ChangeOperConst(GT_CNS_INT);
            op1->AsIntCon()->gtIconVal = vnStore->ConstantValue<int>(vnCns);
+
+            if (vnStore->IsVNHandle(vnCns))


Presumably this case happens only on 32 bit targets?

I was also considering not propagating handle constants into trees as they're generally not foldable and end up producing large immediates.

mikedn · 2019-10-23T19:28:44Z

src/jit/assertionprop.cpp

-                {
-                    // A GT_TRUE is always the last node in a tree, so we can break here
-                    assert((tree->gtNext == nullptr) && (stmt->GetNextStmt() == nullptr));
-                    break;


Hmm, can't remember exactly why I added this early exit for JTRUE nodes. Most likely for consistency with the assertion gen loop that has special handling for JTRUE. Here the only potentially unwanted side effect of not exiting seems to be setting the wrong bit in assertions. But since it's the last node of the block it doesn't matter...

AndyAyersMS · 2019-10-29T16:36:15Z

@dotnet/jit-contrib anyone want to take a look?

briansull

Looks Good, Left some comments

briansull · 2019-10-29T18:07:07Z

src/jit/assertionprop.cpp


-    if (op1->gtOper != GT_LCL_VAR)
+    // Bail out if tree is not side effect free.
+    if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0)


Can you describe the problem that you saw here when GTF_SIDE_EFFECT was true.
Did this change cause any current Asm diffs?

No diffs from the previous version. I added this as a precaution, to parallel what we do above for the =/!= 0 case, since the simplification below will not preserve side effects.

src/jit/assertionprop.cpp

briansull

Handling of OAK_NOT_EQUAL for GT_IND not consistent with comments

briansull · 2019-10-30T22:15:46Z

src/jit/assertionprop.cpp

- */
+//------------------------------------------------------------------------
+// optGlobalAssertionIsEqualOrNotEqual: Look for an assertion in the specified
+//        set that is one of op1 == op1, op1 != op2, *op1 == op2, *op1 != op2,


The code below only allows for *op1 == op2, not *op1 != op2

Thanks. Meant just to handle the equality case.

briansull · 2019-10-30T22:15:49Z

src/jit/assertionprop.cpp

+
+        // Look for matching exact type assertions based on vtable accesses
+        if ((curAssertion->assertionKind == OAK_EQUAL) && (curAssertion->op1.kind == O1K_EXACT_TYPE) &&
+            op1->OperIs(GT_IND))


You don't allow OAK_NOT_EQUAL here.

Right -- let me update the comment.

mikedn reviewed Oct 23, 2019

View reviewed changes

sandreenko added the area-CodeGen label Oct 23, 2019

mikedn reviewed Oct 23, 2019

View reviewed changes

AndyAyersMS added 2 commits October 24, 2019 12:17

Review feedback. Also add a no side effect check.

bdfdf1d

fix unrenamed local

5b43358

briansull approved these changes Oct 29, 2019

View reviewed changes

review feedback

a38f885

briansull reviewed Oct 30, 2019

View reviewed changes

review feedback II

fc31cd1

AndyAyersMS merged commit 5216c8c into dotnet:master Nov 5, 2019

AndyAyersMS deleted the AssertionPropRemoveRedundantExactTypeTest branch November 5, 2019 16:03

Conversation

AndyAyersMS commented Oct 23, 2019

Uh oh!

AndyAyersMS commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Oct 29, 2019

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndyAyersMS Oct 30, 2019 •

edited

Loading